-
Notifications
You must be signed in to change notification settings - Fork 233
Android plugin API updated to support v2 Embedding #486
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
Conversation
@Jeasmine any chance to check this? |
@emawby please can you review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 23 files at r1.
Reviewable status: 1 of 23 files reviewed, 4 unresolved discussions (waiting on @emawby and @luis901101)
a discussion (no related file):
Thank you so much for the contribution this looks great! I would like to clean this PR up to only the required changes if possible. There are a lot of changes to files in .idea and gradle versions. Are all of those changes necessary?
.idea/modules.xml, line 5 at r1 (raw file):
<component name="ProjectModuleManager"> <modules> <module fileurl="file://$PROJECT_DIR$/.idea/onesignal_flutter_official_fork.iml" filepath="$PROJECT_DIR$/.idea/onesignal_flutter_official_fork.iml" />
I don't think we want to commit this change to the main branch
android/build.gradle, line 11 at r1 (raw file):
dependencies { classpath 'com.android.tools.build:gradle:3.5.3'
Is this gradle version bump required for these changes?
android/gradle/wrapper/gradle-wrapper.properties, line 6 at r1 (raw file):
zipStoreBase=GRADLE_USER_HOME zipStorePath=wrapper/dists distributionUrl=https\://services.gradle.org/distributions/gradle-5.6.2-all.zip
Is this gradle version change required?
Nop, those changes you ask are not required at all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luis901101 Appreciate the contribution here! I reviewed your PR as well and the changes to the logic to support the Flutter V2 Android API themselves look good. However as @emawby noted there are unrelated changes to project files, some not compatible with main
.
@luis901101 If you can address those comments and remove any changes you know are not necessary we can merge this PR in. If your not in a good position to make these changes this week let us know and we can make the needed changes.
Ok, I'll remove those changes |
…mpatible with apps that don’t use the v2 Android embedding.
8d2013e
to
952e78c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clean up, looks good to me!
@emawby If you want to take another quick pass at the comments you left and approve one of us can merge it in.
@luis901101 after some change that you pushed we get a failure getting the dependency from github:
It looks like that commit that we were referencing does not exist anymore, maybe you changed the history somehow? |
Yep after I removed unnecessary file changes from PR yesterday that commit is not available any more, the commit now is onesignal_flutter: #^3.2.3
git:
url: https://github.com/luis901101/OneSignal-Flutter-SDK.git
ref: migrate_android_embedding_v2 That way does not matter the commit, it will download the latest commit to that branch which is the one I did yesterday. |
@luis901101 yeah, when you do a It's awesome that the PR was merged! |
Great PR @luis901101, congrats for your efforts. |
Hello, just curious, any update to release a new version from main branch that has this PR so we don't have to target a specific branch? |
As requested here Issue 480, it's necessary to update the Android implementation to support the v2 Embedding which it's the default for new Android plugins and it is highly recommended to be update it because future release of Flutter will fail to build with deprecated Android embedding.
I migrated the Android implementation to support embedding v2 while remaining compatible with apps that don’t use the v2 Android embedding.
This change is