OneSignal / OneSignal-Flutter-SDK

OneSignal is a free push notification service for mobile apps. This plugin makes it easy to integrate your flutter app with OneSignal
https://www.onesignal.com
Other
616 stars 213 forks source link

[chore] Android: move namespace declaration #818

Closed timokz closed 8 months ago

timokz commented 8 months ago

Description

One Line Summary

Enables building apps with agp =< 8.0

Details

Motivation

As this helpful PR already attempted, for apps using Gradle 8.0 or greater, moving the namespace declaration from the AndroidManifest.xml files is necessary. The other PR has a minor syntax error and doesnt delete the conflicting declaration in the xml files.

Otherwise developers will be confronted with this error:

* What went wrong:
A problem occurred evaluating root project 'android'.
> A problem occurred configuring project ':app'.
   > Could not create an instance of type com.android.build.api.variant.impl.ApplicationVariantImpl.
      > Namespace not specified. Please specify a namespace in the module's build.gradle file like so:

        android {
            namespace 'com.example.namespace'
        }

        If the package attribute is specified in the source AndroidManifest.xml, it can be migrated automatically to the namespace value in the build.gradle file using the AGP Upgrade Assistant; please refer to https://developer.android.com/studio/build/agp-upgrade-assistant for more information.

Or other, more obscure warnings thrown by the gradle wrapper.

Scope

None of the actual source is changed, the changes are contained to the android directory of the package and the example app.

Other

I've also updated the AndroidManifest.xml files to have their namespace declaration, e.g. package="com.onesignal.onesignalexample" in the <manifest> tag removed, as the AGP upgrade agent would also refactor to be in line with the newer gradle versions.

Manual testing

Tested building the example and my own app, using flutter sdks 3.16.5 and 3.13.0, respectively. All features in the example app work as intended. They also shouldn't be affected by my changes.

Affected code checklist

None apply, buildscripts are affected.

Checklist

Overview

Testing

Final pass


This change is Reviewable

Raph0007 commented 8 months ago

lgtm 👍🏻

NinoBass commented 8 months ago

Hey @timokz Check in the android/build.gradle file, I think it should be com.onesignal.flutter and not com.onesignal.onesignalexample

nan-li commented 8 months ago

Hi @timokz thank you for submitting this PR, we did approve a contributor's PR for this same issue.

Is the removal of the package name in the Android Manifest file necessary?

timokz commented 8 months ago

Hey @nan-li thanks for the maintenance! Now I can go back to using the original dependence.

Regarding the manifest files: It is recommended and also applied by the AGP upgrade assistant . My pr also includes the upgrade for the example project.

You can verify that by running the assistant in a project of yours or (not ideal) checking this screenshot in the official docs

nan-li commented 8 months ago

Regarding the manifest files: It is recommended and also applied by the AGP upgrade assistant . My pr also includes the upgrade for the example project.

You can verify that by running the assistant in a project of yours or (not ideal) checking this screenshot in the official docs

Ahh ok, thank you @timokz , I understand!

I don't think we are ready to port our projects over to AGP 8, since it will certainly break some colleagues' setups.

We have released the namespace support for the OneSignal Flutter SDK on both Release 3.5.2 and Release 5.1.0, so I will close this PR.

Thanks again for submitting this PR!