Skip to content

feat: [SDK-4774] support Unity no-location builds#877

Closed
fadi-george wants to merge 21 commits into
mainfrom
fadi/sdk-4774
Closed

feat: [SDK-4774] support Unity no-location builds#877
fadi-george wants to merge 21 commits into
mainfrom
fadi/sdk-4774

Conversation

@fadi-george

Copy link
Copy Markdown
Collaborator

Description

One Line Summary

Adds a no-location Unity demo and a robust flag-controlled native location dependency opt-out for Unity SDK builds.

Details

Motivation

Apps that do not use OneSignal location APIs should be able to exclude the native location module from iOS and Android builds. This also aligns Unity with the .NET, Capacitor, and Cordova SDK opt-out patterns while avoiding mutation of shared SDK package files during dependency resolution.

Scope

  • Adds ONESIGNAL_DISABLE_LOCATION support layered over the existing Unity Editor setting, with the environment variable taking precedence for CLI and CI builds.
  • Generates EDM4U dependency manifests per Unity project under Assets/OneSignal/Editor instead of rewriting shared package Editor files.
  • Updates iOS post-processing for no-location builds, notification service extension dependencies, and CocoaPods duplicate output handling.
  • Adds and documents examples/demo-no-location, including iOS/Android scripts, build settings, no-location native dependency sets, stripping preservation, and a test notification flow.
  • Improves missing location module behavior by logging SDK-level errors when location APIs are called without the native location module.

Testing

Unit testing

Not added; these changes are primarily Unity Editor build pipeline, native dependency resolution, and demo build behavior.

Manual testing

  • examples/demo-no-location/run-ios.sh --no-install generated granular iOS pods without OneSignalLocation.
  • xcodebuild for examples/demo-no-location iOS simulator completed with EXIT=0; verified no OneSignalLocation.framework was linked.
  • examples/demo/run-ios.sh --no-install generated umbrella iOS and Android dependency manifests for the main demo.
  • examples/demo-no-location/run-android.sh --no-install built the APK with granular Android dependencies (core, notifications, in-app-messages).
  • ./run-all.sh --sdk=unity --platform=ios passed: 34 tests passing.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

Made with Cursor

fadi-george and others added 21 commits June 29, 2026 14:28
The scripted iOS/Android builds use ManagedStrippingLevel.High, which
stripped the reflection-only OneSignaliOS/OneSignalAndroid platform
implementations. _getDefaultInstance() then threw "Sequence contains no
elements", so OneSignal never initialized and the permission prompt never
appeared. Add a link.xml (matching the main demo) to preserve the
OneSignal assemblies, and align the demo UI with the .NET no-location
demo (Permission + Push ID rows).

Co-authored-by: Cursor <cursoragent@cursor.com>
Explain that Assets/OneSignal/link.xml must not be deleted: scripted
builds use high managed stripping and the OneSignal platform classes are
reflection-resolved, so removing it breaks SDK initialization.

Co-authored-by: Cursor <cursoragent@cursor.com>
Adds a "Send Test Notification" button that posts to the OneSignal REST
API targeting the current push subscription, matching the .NET and
Capacitor no-location demos. Enables the UnityWebRequest module needed
for the request.

Co-authored-by: Cursor <cursoragent@cursor.com>
@fadi-george

Copy link
Copy Markdown
Collaborator Author

@claude review once

Comment on lines 109 to 116
sleep 30
done
if [ "$FOUND" != "true" ]; then echo "✗ Android SDK version ${VERSION} not found after 5 attempts"; exit 1; fi
sed -i -E "s/spec=\"com\.onesignal:OneSignal:[0-9][0-9.]*\"/spec=\"com.onesignal:OneSignal:$VERSION\"/" com.onesignal.unity.android/Editor/OneSignalAndroidDependencies.xml
sed -i -E "s/const string Version = \"[0-9][0-9.]*\"/const string Version = \"$VERSION\"/" com.onesignal.unity.android/Editor/OneSignalAndroidDependencies.cs
sed -i -E "s/<androidPackage spec=\"com\.onesignal:OneSignal:[0-9][0-9.]*\"/<androidPackage spec=\"com.onesignal:OneSignal:$VERSION\"/" examples/demo/Assets/OneSignal/Editor/OneSignalAndroidDependencies.xml
sed -i -E "s/'com\.onesignal:OneSignal:[0-9][0-9.]*'/'com.onesignal:OneSignal:$VERSION'/" examples/demo/Assets/Plugins/Android/mainTemplate.gradle
sed -i -E "s/<package>com\.onesignal:OneSignal:[0-9][0-9.]*<\/package>/<package>com.onesignal:OneSignal:$VERSION<\/package>/" examples/demo/ProjectSettings/AndroidResolverDependencies.xml

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 The release workflow in .github/workflows/create-release-pr.yml only updates files under examples/demo/ and never touches the new examples/demo-no-location/ project, so the four committed dependency files there (Assets/OneSignal/Editor/OneSignal{Android,iOS}Dependencies.xml, Assets/Plugins/Android/mainTemplate.gradle, ProjectSettings/AndroidResolverDependencies.xml) will retain stale versions after each release. Additionally, the existing sed patterns only match the umbrella com.onesignal:OneSignal:VERSION artifact and would not match this demo's granular artifacts (core/notifications/in-app-messages, OneSignalXCFramework/OneSignal\[InAppMessages\]) even if the paths were added. Fix needs both path additions and new sed patterns that target the granular coordinate forms.

Extended reasoning...

What the bug is. This PR adds a new demo project at examples/demo-no-location/ and checks in four files that hardcode the OneSignal native SDK versions (currently 5.9.5 for Android, 5.5.3 for iOS). The release workflow at .github/workflows/create-release-pr.yml is responsible for bumping these hardcoded versions in the umbrella examples/demo/ project on every release, but its sed steps only enumerate examples/demo/ paths — examples/demo-no-location/ is never touched.

The specific code path. Lines 113-115 (Android) update examples/demo/Assets/OneSignal/Editor/OneSignalAndroidDependencies.xml, examples/demo/Assets/Plugins/Android/mainTemplate.gradle, and examples/demo/ProjectSettings/AndroidResolverDependencies.xml. Line 137 (iOS) updates examples/demo/Assets/OneSignal/Editor/OneSignaliOSDependencies.xml. None of these touch examples/demo-no-location/. Beyond the missing paths, the regexes themselves are umbrella-artifact-only: com\.onesignal:OneSignal:[0-9][0-9.]* matches com.onesignal:OneSignal:5.9.5 but does not match the granular forms used by the no-location demo (com.onesignal:core:5.9.5, com.onesignal:notifications:5.9.5, com.onesignal:in-app-messages:5.9.5).

Why existing code doesn't prevent it. The new OneSignalAndroidDependencies.cs / OneSignaliOSDependencies.cs files do regenerate the XML manifests via [InitializeOnLoad] when Unity opens the project, so the two XMLs under Assets/OneSignal/Editor/ will eventually self-heal — but only once a developer opens demo-no-location in the Unity Editor. The release workflow runs in CI without Unity, so the committed release state still ships stale XML. More importantly, Assets/Plugins/Android/mainTemplate.gradle and ProjectSettings/AndroidResolverDependencies.xml are EDM4U snapshots that only refresh on an explicit Force Resolve, which is not triggered by anything in the release pipeline.

Impact. A release bump produces a commit where examples/demo/ is at the new version and examples/demo-no-location/ is still at the previous version, across both Android (gradle + resolver xml) and iOS (pod manifest). Anyone checking out the release tag sees inconsistent demo state. SDK consumers installing via UPM are unaffected — this is a release-engineering / demo-hygiene correctness issue, not a runtime bug.

Step-by-step proof. Suppose the next release is invoked with android_version=5.10.0, ios_version=5.6.0:

  1. Workflow updates com.onesignal.unity.android/Editor/OneSignalAndroidDependencies.cs Version const → 5.10.0. ✓
  2. Workflow runs sed -i -E "s/<androidPackage spec=\"com\.onesignal:OneSignal:[0-9][0-9.]*\"/.../" examples/demo/Assets/OneSignal/Editor/OneSignalAndroidDependencies.xml → matches and rewrites. ✓
  3. Workflow does NOT run sed on examples/demo-no-location/Assets/OneSignal/Editor/OneSignalAndroidDependencies.xml. The file still reads com.onesignal:core:5.9.5, com.onesignal:notifications:5.9.5, com.onesignal:in-app-messages:5.9.5. ✗
  4. Even if the path were added, the regex com\.onesignal:OneSignal:[0-9][0-9.]* does not match com.onesignal:core:5.9.5 (artifact id core, not OneSignal). Still stale. ✗
  5. Same outcome for examples/demo-no-location/Assets/Plugins/Android/mainTemplate.gradle (implementation 'com.onesignal:core:5.9.5' etc.) and examples/demo-no-location/ProjectSettings/AndroidResolverDependencies.xml (<package>com.onesignal:core:5.9.5</package> etc.). ✗
  6. The release commit lands with examples/demo/ updated to 5.10.0/5.6.0 and examples/demo-no-location/ stuck at 5.9.5/5.5.3.

How to fix. Extend the Android step to also run granular-artifact substitutions across the demo-no-location files, e.g.:

for artifact in core notifications in-app-messages; do
  sed -i -E "s/(com\.onesignal::)[0-9][0-9.]*/\1$VERSION/g" \
    examples/demo-no-location/Assets/OneSignal/Editor/OneSignalAndroidDependencies.xml \
    examples/demo-no-location/Assets/Plugins/Android/mainTemplate.gradle \
    examples/demo-no-location/ProjectSettings/AndroidResolverDependencies.xml
done

And the iOS step needs to update examples/demo-no-location/Assets/OneSignal/Editor/OneSignaliOSDependencies.xml — the existing version="[0-9][0-9.]*" regex would actually match the granular OneSignalXCFramework/OneSignal and OneSignalXCFramework/OneSignalInAppMessages iosPod entries, so just adding the path to line 137 is sufficient on the iOS side.

Comment on lines 55 to 81
public bool IsShared
{
get => _location.Call<bool>("isShared");
set => _location.Call("setShared", value);
get
{
try
{
return _location != null && _location.Call<bool>("isShared");
}
catch (Exception)
{
SDKDebug.Error(LocationModuleNotAvailable);
return false;
}
}
set
{
try
{
if (_location != null)
_location.Call("setShared", value);
}
catch (Exception)
{
SDKDebug.Error(LocationModuleNotAvailable);
}
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 On Android, when the native location module is missing, IsShared getter/setter silently no-op without logging because the _location != null short-circuit skips the try-block before SDKDebug.Error can fire. This is inconsistent with RequestPermission in the same file (which always logs via the caught NRE) and with the iOS bridge in OneSignalUnityBridgeLocation.mm (which logs on every call). Suggest dropping the null guards (let the NRE catch and log, matching RequestPermission) or adding an explicit if (_location == null) { SDKDebug.Error(LocationModuleNotAvailable); ... } branch in each accessor.

Extended reasoning...

Bug: In com.onesignal.unity.android/Runtime/AndroidLocationManager.cs the constructor catches the missing-module exception from sdkClass.CallStatic<AndroidJavaObject>("getLocation"), logs LocationModuleNotAvailable once, and leaves _location = null. After that, the IsShared accessors are guarded with a null short-circuit:

  • Getter (line 61): return _location != null && _location.Call<bool>("isShared"); — when _location == null, C# short-circuits the && and returns false without entering the try body, so SDKDebug.Error is never invoked.
  • Setter (lines 73-74): if (_location != null) _location.Call("setShared", value); — same short-circuit; nothing is logged.
  • RequestPermission (line 88): unconditionally dereferences _location.Call(...), so a null _location throws a NullReferenceException that the catch block converts into SDKDebug.Error(LocationModuleNotAvailable) on every call.

Why existing code doesn't prevent it: the try/catch around each accessor only catches the JNI exception that would come from a real Call(...) on a non-null _location. The null guard added in this PR sits outside that path, so no exception is ever thrown for the null branch and the diagnostic log added in the same change is unreachable for the IsShared accessors.

Impact: the PR description says it 'logs SDK-level errors when location APIs are called without the native location module', and the iOS bridge (OneSignalUnityBridgeLocation.mm) enforces that contract by logging OneSignalLocationModuleNotFoundMessage on every _oneSignalLocationGetIsShared, _oneSignalLocationSetIsShared, and _oneSignalLocationRequestPermission call via the ONESIGNAL_DISABLE_LOCATION macro. On Android, RequestPermission matches that contract but the IsShared accessors are silent for every call after construction. A developer toggling off the module and trying to debug why OneSignal.Location.IsShared = true had no effect on Android would see only the one boot-time error, while the iOS counterpart would emit an API-call-site error each time. This is a cross-platform diagnostic divergence rather than a functional break — the no-op + false return value documented in the README still holds.

Step-by-step proof: consider the no-location demo (which this PR adds) with disableLocation = true. (1) AndroidOneSignal.Initialize constructs AndroidLocationManager. (2) sdkClass.CallStatic<AndroidJavaObject>("getLocation") throws because the com.onesignal:location module is absent; the constructor's catch logs once. (3) The app reads OneSignal.Location.IsShared once per frame to display state. (4) Each call enters the getter, evaluates _location != null to false, short-circuits, returns false. No Call is made, no exception, the catch block is dead code for this path. (5) On iOS, the same loop hits _oneSignalLocationGetIsShared, which goes through #if ONESIGNAL_DISABLE_LOCATION and logs every frame. Result: same code path, divergent diagnostic output.

Addressing the refutation: the refuter is right that (a) the constructor already logs once, (b) the README documents the no-op contract, and (c) logging every frame can be argued as spam. Those are fair points — which is why this is filed as a nit, not normal severity. But the PR's own description and the iOS implementation set the precedent that each API call without the module produces a log, and RequestPermission in this same file already honors that contract via the NRE catch. The Android IsShared accessors are the only callers that diverge. If log-once-at-boot is the intended contract, the iOS bridge and RequestPermission should be updated to match; otherwise IsShared should follow them. Either resolution works — the current state where three of four call sites log and one doesn't is the issue.

Fix: remove the null short-circuit so the NRE path catches and logs (matching RequestPermission), or add an explicit early-return:

get {
    if (_location == null) {
        SDKDebug.Error(LocationModuleNotAvailable);
        return false;
    }
    try { return _location.Call<bool>("isShared"); }
    catch (Exception) { SDKDebug.Error(LocationModuleNotAvailable); return false; }
}

@nan-li nan-li self-requested a review June 30, 2026 20:34
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.

1 participant