feat: [SDK-5083] add an inert-by-default notification service extension to the demo - #2726
feat: [SDK-5083] add an inert-by-default notification service extension to the demo#2726nan-li wants to merge 3 commits into
Conversation
…on to the demo Nothing in the repo implemented INotificationServiceExtension, so reproducing an NSE bug meant writing one from scratch and no compiled sample guarded the interface against a breaking change. Building the demo inside OneSignalSDK's :app project now turns that into a CI failure, and the release build exercises the -keep rule in onesignal/notifications/consumer-rules.pro end to end. Six switches drive it, all off, folded behind a Show options row. It reads them from SharedPreferences rather than MainViewModel because it runs whether or not the app is open, and it sets an extender only when a switch needs one, since an extender makes the SDK display a data-only push carrying no alert. The channel readout uses NotificationCompat.getChannelId inside the extender, the only place an extension sees the SDK's choice. A restored notification lands on restored_OS_notifications whatever the payload asked for. Logging restoring next to it waits on SDK-5011.
Demo output and forwarded SDK output sat side by side in logcat under tags that gave no hint which was which, which made reading a notification repro slower than it needed to be. DemoLog stamps both the tag and the message, so `logcat -s` still filters on the tag and a line stays recognizable when only the message column is in view. Callers pass the plain class name and DemoLog adds the prefix, keeping [Demo] in one place. All 127 demo call sites go through it. The five forwarding calls in MainApplication keep using android.util.Log and stay unmarked. Those lines are the SDK's, and marking them would bury the demo's own output whenever you grep [Demo].
📊 Diff Coverage Report✓ Coverage check passed (no source files changed) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Multi-model review (Opus 5 / GPT 5.6 Sol / Grok 4.6) of the demo NSE work. Demo-only; no SDK source issues.
Act on
- Delay Display can drop the notification. Raw
Thread+Thread.sleepwith notry/finally. If the thread is interrupted or dies,display()never runs and the SDK’s 30s waiter treats it aswantsToDisplay = false.MainApplication’s existing delay path already catchesInterruptedExceptionand still callsdisplay(). - Documented extender / data-only-push behavior is wrong for 5.x.
processHandlerResponsecheckscanDisplay(nonemptyalert) beforeshouldDisplayNotification. An extender cannot make a bodyless push display. Conditional extender is still reasonable; the rationale in the class andbuild.mdis not.
Consider
- Foreground Delay Display stacks with
MainApplication’s 2s lifecycle delay (~7s observed). [Demo]OneSignalRepositoryis 25 chars; tags over 23 throw on API 21–23 (minSdk21).- Hidden Discard/Delay stay live after collapse/rotation with no visible indicator.
Noted / dismissed
- Preference backup restoring NSE switches, title-prefix clobbering the app-name default, leftover high-importance channel, and
build.mdstaleLog.*mentions: lone-model or low-impact demo nits.
Sent by Cursor Automation: PR Reviews
The comment claimed an extender makes the SDK display a data-only push, so installing a no-op one would not be inert. That is not how 5.x behaves. processHandlerResponse gates on canDisplay, a non-empty notification body, before it reaches shouldDisplayNotification, so hasExtender() is never read for a bodyless push and an extender cannot rescue one. The code stays as it is. Setting an extender only when a switch needs one is still right, just for the duller reason that nothing asked for it otherwise. Left uncorrected, a customer reading the demo could design around SDK behavior that does not exist.
|
I would check this doesnt break the e2e tests suit, you can test with sdk-shared run-local |
fadi-george
left a comment
fadi-george
left a comment
There was a problem hiding this comment.
Ideally we dont modify the current demo and just maybe make another example e.g. examples/demo-nse
The NSE is disabled by default, do you think we really need another demo app just for the NSE? 
|
Done locally, all passed. Note that I don't think the e2e workflow itself added in #2652 has ever ran |
The demo app is supposed to almost the same for all the wrappers. Ideally we don't add stuff to it willy-nilly. I still think this could be a separate example i.e. examples/demo-nse it can just have the section you've made and maybe some test notification if it needs it. |
2 participants
Description
One Line Summary
Adds a notification service extension to the Android demo that stays inert until you switch it on.
Details
Motivation
Nothing in this repo implements
INotificationServiceExtension. Reproducing an extension bug means writing one from scratch every time. And because the SDK discovers the class by reflection from manifest meta-data, a breaking change toINotificationServiceExtensionorINotificationReceivedEventstill compiles and only fails at runtime. A demo implementation compiling in CI closes both gaps.Scope
examples/demoonly. No SDK source changes and no public API changes.Everything is off by default. An always-on extension would change the baseline for every notification the demo sends, so anyone chasing a grouping or channel bug would be debugging demo code without realizing it. With every switch off the demo behaves exactly as it did before.
DemoNotificationServiceExtensionis registered from the demo manifest undercom.onesignal.NotificationServiceExtension. Its switches live in a new section, folded behind a Show options row so the section stays two rows tall while the extension is off.nse_enabled_toggleonNotificationReceivedreturns immediately.nse_options_togglense_log_togglense_extender_toggleNotificationCompat.Extender.nse_high_importance_toggleIMPORTANCE_HIGHchannel.nse_delay_togglepreventDefault(), thendisplay()five seconds later.nse_discard_togglepreventDefault(true). Takes precedence over the switches above.Switches persist through
SharedPreferenceUtil. The extension reads them from there rather thanMainViewModel, since it runs whether or not the app is open.Design notes
The extender is set conditionally, only when Log Details, Apply Extender, or Force High Importance Channel is on, rather than installed as a no-op whenever the master switch is on. That is about not doing work nothing asked for. It does not change what displays.
NotificationGenerationProcessor.shouldDisplayNotificationdoes readhasExtender(), butprocessHandlerResponsegates oncanDisplay, a non-empty notification body, before it gets there, so an extender cannot rescue a bodyless push.The channel readout happens inside the extender.
INotificationexposes no channel, so the demo reads it back withNotificationCompat.getChannelId(builder.build())from insideextend(builder), where the SDK has already applied the channel it resolved. ReadingchnloffrawPayloadwould report what the payload asked for rather than what the SDK chose, and the two diverge on a restore.NotificationChannelManager.createNotificationChannelreturnsrestored_OS_notificationsbefore it ever looks atchnl, while the stored payload is identical to the first delivery.event.restoringis not onINotificationReceivedEventyet, so the log line omits it. #2723 adds it, and a TODO in the class marks the spot.Log tags
Second commit, separable from the first. Demo output and forwarded SDK output sat side by side in logcat under tags that gave no hint which was which.
DemoLogstamps both the tag and the message.Callers pass the plain class name and
DemoLogadds the prefix, so[Demo]is defined in one place. All 127 demo call sites go through it. The five forwarding calls inMainApplicationkeepandroid.util.Logand stay unmarked, since those lines belong to the SDK and marking them would bury the demo's own output whenever you grep[Demo].Testing
Unit testing
None added. This is demo code with no test target, and the compile is the guard.
ci.yml'sdemo-buildjob builds:appfrom insideOneSignalSDK/, wheresettings.gradlesubstitutes the published dependency with local source, so a breaking change to either interface now fails the build.The release build also exercises the
-keep class ** implements com.onesignal.notifications.INotificationServiceExtensionrule inonesignal/notifications/consumer-rules.proend to end for the first time. I confirmed the class survives R8 mapped to itself and that the meta-data lands in the merged release manifest.Manual testing
Pixel 7 emulator, API 34, Google Play image,
gmsdebug build against local SDK source.fcm_fallback_notification_channel, title unchanged, nothing logged[NSE] Simple Notificationdemo_nse_high_importance,mImportance=4preventDefault(false)at 13:10:10.183, display at 13:10:15.188preventDefault(true), nothing postedKilling the app process with
adb shell am killand then sending a push showed the extension firing in a fresh pid, reading its switches from SharedPreferences with noMainViewModelalive.After
adb rebootthe same notifications came back through the extension loggingchannel=restored_OS_notificationsatimportance=2, againstfcm_fallback_notification_channelatimportance=3on first delivery. The payload is identical in both cases, which is why the channel is read from the builder.spotlessCheck,detekt,assembleGmsRelease, andcompileHuaweiDebugKotlinall pass.Affected code checklist
Ticked because the demo extension exercises those paths. No SDK behavior changes.
Checklist
Overview
Testing
Final pass