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

Add xamarin way to use OSRemoteNotificationReceivedHandler #381

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tmijieux
Copy link

@tmijieux tmijieux commented Dec 2, 2023

Description

One Line Summary

Add an easy way to use the OSRemoteNotificationReceivedHandler interface to replace old NotificationExtender in Xamarin .

Details

Usage in xamarin project:
In manifest:

    <meta-data
        android:name="com.onesignal.NotificationServiceExtension"
        android:value="com.my.app.MyOneSignalServiceExtension" />
using System;
using Android.Content;
using AndroidX.Core.App;
using Com.OneSignal.Android;

namespace MyApp.Droid
{
    public class MyExtender : NotificationExtenderBase
    {
        public override NotificationCompat.Builder Extend(NotificationCompat.Builder builder)
        {
            //do something with builder
            return builder
                /*.AddAction(new NotificationCompat.Action(null, "coucou", null))*/
                ;
        }
    }

    [NotificationExtension(Name = "com.my.app.MyOneSignalServiceExtension")]
    public class NotificationExtender : OSRemoteNotificationReceivedBase
    {
        public override void RemoteNotificationReceived(Context _ctx, OSNotificationReceivedEvent ev)
        {
            var notif = ev.Notification.MutableCopy();
            notif.SetExtender(new MyExtender());

            if (notif?.AdditionalData == null)
            {
                ev.Complete(notif);
                return;
            }



            bool hideNotif = false;
            try
            {
                var discard = notif.AdditionalData.Get("silent");
                if (discard != null)
                {
                    hideNotif = true;
                }
            }
            catch (Exception /*e*/)
            {
            }
            ev.Complete(hideNotif ? null : notif);
        }
    }
}

I have made the following modification on the native android project:
OneSignal/OneSignal-Android-SDK@main...tmijieux:OneSignal-Android-SDK:use_OSRemoveNotificationReceivedHandler_in_xamarin

I'm not 100% sure the java code is required. I think we need the abstract base classes because if we just use the interface directly from csharp i'm not sure if there will be a peer object /peer class in java world resolvable with introspection.
Maybe it works with just the interface ? i will try it tomorrow.

NOTE: to make the native android code compile i had to upgrade minSdkVersion to 19 , you should probably recompile the native code on your side with your requirements

Motivation

Since the update to 4.x it was unclear how to replace the NotificationExtenderService

Scope

This is only an extension, no existing feature should be affected.

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
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • 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.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

@tmijieux tmijieux changed the title add abstract base class in java world to access OSRemoteNotificationR… [WIP] add abstract base class in java world to access OSRemoteNotificationR… Dec 2, 2023
@tmijieux tmijieux force-pushed the use_OSRemoveNotificationReceivedHandler_in_xamarin branch from 6693065 to c38ccc1 Compare December 2, 2023 01:54
@tmijieux tmijieux changed the title [WIP] add abstract base class in java world to access OSRemoteNotificationR… Add abstract base class in java world to access OSRemoteNotificationR… Dec 2, 2023
@brismithers
Copy link
Contributor

Overall this solution makes sense! Are those native Android changes required? If so, I would like to get that PR in as well. Also, note this change would be valuable to v5 of the SDK (which is already on monoandroid12.0). Thanks!

@tmijieux tmijieux force-pushed the use_OSRemoveNotificationReceivedHandler_in_xamarin branch from c38ccc1 to 77f99bb Compare December 5, 2023 15:18
@tmijieux
Copy link
Author

tmijieux commented Dec 5, 2023

I just updated my commit to put the v12 in all of the three places you mentioned.

Are those native Android changes required ?

I just tested and It seems to work fine with the following code but it is probably a little bit harder/awkward to setup since because of the nested interface we are required to use, I think We can ditch the modification on the native code, but to keep the initial proposed usage with the simplicity of the single base class we can add these classes in the additions part of the android binding library. would that be ok ?

using System;
using System.Diagnostics;
using Android.Content;
using AndroidX.Core.App;
using Com.OneSignal.Android;
using MyApp;
using MyApp.Services;
using MyApp.Helpers;

using IExtender = AndroidX.Core.App.NotificationCompat.IExtender;

namespace MyApp.Droid
{
    public class MyExtender : Java.Lang.Object, IExtender
    {
        public  NotificationCompat.Builder Extend(NotificationCompat.Builder builder)
        {
            //do something with builder
            return builder
                //.SetContentTitle("mytest1")
                //.SetContentText("mytest2")
                //.AddAction(new NotificationCompat.Action(null, "mytest3", null))
                ;
        }
    }

    [NotificationExtension(Name = "com.my.app.MyOneSignalServiceExtension")]
    public class NotificationExtender : Java.Lang.Object, OneSignal.IOSRemoteNotificationReceivedHandler
    {
        public void RemoteNotificationReceived(Context _ctx, OSNotificationReceivedEvent ev)
        {
            Debug.WriteLine("In my extender!");
            var notif = ev.Notification.MutableCopy();
            notif.SetExtender(new MyExtender());

            if (notif?.AdditionalData == null)
            {
                Debug.WriteLine("regular notif");
                ev.Complete(notif);
                return;
            }

            bool hideNotif = false;
            try
            {
                var discard = notif.AdditionalData.Get("silent");
                if (discard != null)
                {
                    Debug.WriteLine("silent!");
                    hideNotif = true;
                }
            }
            catch (Exception /*e*/)
            {
            }
            ev.Complete(hideNotif ? null : notif);
        }
    }
}

@tmijieux tmijieux force-pushed the use_OSRemoveNotificationReceivedHandler_in_xamarin branch 3 times, most recently from 6b4deff to 88d5367 Compare December 5, 2023 16:29
@tmijieux
Copy link
Author

tmijieux commented Dec 5, 2023

The last version i pushed should work (tested on my side) and I removed the modification on the aar archive, with the same usage as initially proposed.

@tmijieux tmijieux changed the title Add abstract base class in java world to access OSRemoteNotificationR… Add xamarin way to use OSRemoteNotificationReceivedHandler Dec 5, 2023
@brismithers
Copy link
Contributor

what you did makes sense, thanks!

Copy link
Contributor

@brismithers brismithers left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 5 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @emawby, @jinliu9508, @jkasten2, and @tmijieux)

@tmijieux
Copy link
Author

Hi. Is this PR still in review ? gitlab still list this as "change requested" when i addressed everything .

@brismithers brismithers requested review from brismithers and removed request for brismithers May 13, 2024 17:13
This permits to use the remoteNotificationReceived mechanism in xamarin
to be able to customize notification behavior and display when the app is swiped

The reference to androidx is required in order to bring in
some androidx types that were missing
and prevented the generation of some API
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.

2 participants